Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

iox-#2044 Refactor fixed position container and move to dust #2051

Merged
merged 9 commits into from
Oct 25, 2023
Merged

iox-#2044 Refactor fixed position container and move to dust #2051

merged 9 commits into from
Oct 25, 2023

Conversation

elBoberido
Copy link
Member

@elBoberido elBoberido commented Oct 21, 2023 •
edited
Loading

Pre-Review Checklist for the PR Author

  1. Code follows the coding style of CONTRIBUTING.md
  2. Tests follow the best practice for testing
  3. Changelog updated in the unreleased section including API breaking changes
  4. Branch follows the naming format (iox-123-this-is-a-branch)
  5. Commits messages are according to this guideline
  6. Update the PR title
    • Follow the same conventions as for commit messages
    • Link to the relevant issue
  7. Relevant issues are linked
  8. Add sensible notes for the reviewer
  9. All checks have passed (except task-list-completed)
  10. All touched (C/C++) source code files from iceoryx_hoofs are added to ./clang-tidy-diff-scans.txt
  11. Assign PR to reviewer

Notes for Reviewer

I'm currently trying to replace the keep-alive mechanism from an IPC message to a timestamp in the shared memory. This is another preparatory PR which adds a FixedPositionContainer to replace the current stop gap solution.

I also noticed that the dust tests were not running in the CI and added them.

Checklist for the PR Reviewer

  • Commits are properly organized and messages are according to the guideline
  • Code according to our coding style and naming conventions
  • Unit tests have been written for new behavior
  • Public API changes are documented via doxygen
  • Copyright owner are updated in the changed files
  • All touched (C/C++) source code files from iceoryx_hoofs have been added to ./clang-tidy-diff-scans.txt
  • PR title describes the changes

Post-review Checklist for the PR Author

  1. All open points are addressed and tracked via issues

References

@elBoberido elBoberido force-pushed the iox-2044-refactor-fixed-position-container-and-move-to-dust branch 4 times, most recently from b47edcf to b20e077 Compare October 21, 2023 14:59
@elBoberido elBoberido changed the base branch from master to iox-2041-move-test-class-for-ctor-and-assignment-operator-to-hoofs-testing October 21, 2023 15:51
@elBoberido
Copy link
Member Author

@FerdinandSpitzschnueffler @mossmaurice @dkroenke @MatthiasKillat @elfenpiff @budrus feel free to add yourself as reviewer

During the rewrite of the FixedPositionContainer I noticed that there is an overlap with the ObjectPool. So I tried to refactor the ObjectPool but noticed soon that it essentially would end up in a total rewrite anyway since it uses internally pointer which prevented the use in the shared memory and other issues. Therefore I continued with the FixedPositionContainer. The next PR will remove the FixedPositionContainer from PortPool and also the FixedSizeContainer from PortIntrospection. It would also be possible to replace the ObjectPool in the gateway with the new container. I could do this also in the follow up PR.

@elBoberido elBoberido self-assigned this Oct 21, 2023
@elBoberido elBoberido added enhancement New feature refactoring Refactor code without adding features labels Oct 21, 2023
@elBoberido elBoberido mentioned this pull request Oct 21, 2023
Closed
@elBoberido elBoberido changed the base branch from iox-2041-move-test-class-for-ctor-and-assignment-operator-to-hoofs-testing to master October 22, 2023 16:37
@elBoberido elBoberido force-pushed the iox-2044-refactor-fixed-position-container-and-move-to-dust branch from 29dedf0 to 2ad2e5a Compare October 22, 2023 16:41
@elBoberido elBoberido changed the base branch from master to iox-2041-move-test-class-for-ctor-and-assignment-operator-to-hoofs-testing October 22, 2023 16:42
@elBoberido elBoberido changed the base branch from iox-2041-move-test-class-for-ctor-and-assignment-operator-to-hoofs-testing to master October 23, 2023 19:04
@elBoberido elBoberido force-pushed the iox-2044-refactor-fixed-position-container-and-move-to-dust branch from 9c860b0 to b0c2b66 Compare October 23, 2023 19:04
@elBoberido elBoberido changed the base branch from master to iox-2041-move-test-class-for-ctor-and-assignment-operator-to-hoofs-testing October 23, 2023 19:06
elfenpiff
elfenpiff previously approved these changes Oct 24, 2023
View reviewed changes
doc/website/release-notes/iceoryx-unreleased.md Show resolved Hide resolved
@elBoberido elBoberido force-pushed the iox-2041-move-test-class-for-ctor-and-assignment-operator-to-hoofs-testing branch from 3b56e28 to 91ec2be Compare October 24, 2023 14:32
@elBoberido elBoberido force-pushed the iox-2044-refactor-fixed-position-container-and-move-to-dust branch from b0c2b66 to d9c3857 Compare October 24, 2023 14:37
Base automatically changed from iox-2041-move-test-class-for-ctor-and-assignment-operator-to-hoofs-testing to master October 24, 2023 17:43
@elBoberido elBoberido dismissed elfenpiff’s stale review October 24, 2023 17:43

The base branch was changed.

@elBoberido elBoberido force-pushed the iox-2044-refactor-fixed-position-container-and-move-to-dust branch from d9c3857 to 66d544d Compare October 24, 2023 17:48
@codecov
Copy link

codecov bot commented Oct 24, 2023 •
edited
Loading

Codecov Report

Merging #2051 (66d544d) into master (c5f549f) will increase coverage by 5.76%.
Report is 10 commits behind head on master.
The diff coverage is 97.53%.

❗ Current head 66d544d differs from pull request most recent head 17657f4. Consider uploading reports for the commit 17657f4 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2051      +/-   ##
==========================================
+ Coverage   74.44%   80.20%   +5.76%     
==========================================
  Files         417      419       +2     
  Lines       15986    16148     +162     
  Branches     2272     2291      +19     
==========================================
+ Hits        11900    12952    +1052     
+ Misses       3304     2380     -924     
- Partials      782      816      +34
Flag Coverage Δ
unittests 79.99% <97.53%> (+5.77%) ⬆️
unittests_timing 15.16% <0.00%> (-0.17%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...container/include/iox/fixed_position_container.hpp 100.00% <100.00%> (ø)
...st/vocabulary/include/iox/detail/span_iterator.hpp 68.42% <ø> (+68.42%) ⬆️
...esign/include/iox/detail/newtype/decrementable.hpp 100.00% <ø> (ø)
...esign/include/iox/detail/newtype/incrementable.hpp 100.00% <ø> (ø)
...er/include/iox/detail/fixed_position_container.inl 96.72% <96.72%> (ø)

... and 28 files with indirect coverage changes

@elBoberido elBoberido requested a review from elfenpiff October 24, 2023 17:49
elfenpiff
elfenpiff previously approved these changes Oct 24, 2023
View reviewed changes
@elBoberido elBoberido changed the title [Stacked on #2042] iox-#2044 Refactor fixed position container and move to dust iox-#2044 Refactor fixed position container and move to dust Oct 24, 2023
FerdinandSpitzschnueffler
FerdinandSpitzschnueffler reviewed Oct 25, 2023
View reviewed changes
Copy link
Contributor

@FerdinandSpitzschnueffler FerdinandSpitzschnueffler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, only some nitpicks.

iceoryx_dust/container/include/iox/fixed_position_container.hpp Outdated Show resolved Hide resolved
iceoryx_dust/container/include/iox/fixed_position_container.hpp Outdated Show resolved Hide resolved
iceoryx_dust/container/include/iox/detail/fixed_position_container.inl Outdated Show resolved Hide resolved
iceoryx_dust/container/include/iox/detail/fixed_position_container.inl Outdated Show resolved Hide resolved
iceoryx_dust/test/moduletests/test_container_fixed_position_container.cpp Outdated Show resolved Hide resolved
iceoryx_dust/test/moduletests/test_container_fixed_position_container.cpp Show resolved Hide resolved
@elBoberido elBoberido merged commit 811bd76 into master Oct 25, 2023
14 of 15 checks passed
@elBoberido elBoberido deleted the iox-2044-refactor-fixed-position-container-and-move-to-dust branch October 25, 2023 15:18
@elBoberido elBoberido linked an issue Oct 25, 2023 that may be closed by this pull request
Refactor 'FixdePositionContainer' and move to 'dust' #2044
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FerdinandSpitzschnueffler FerdinandSpitzschnueffler FerdinandSpitzschnueffler approved these changes

@elfenpiff elfenpiff elfenpiff approved these changes

Assignees

@elBoberido elBoberido

Labels
enhancement New feature refactoring Refactor code without adding features
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor 'FixdePositionContainer' and move to 'dust'
3 participants
@elBoberido @elfenpiff @FerdinandSpitzschnueffler